-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust tests #96
Rust tests #96
Conversation
…itai_struct_tests into tuning_kst_generation
…itai_struct_tests into tuning_kst_generation
@Agile86 @revitalyr I thank you for your work, but I will now take over and kindly ask you not to proceed with any changes. I have no doubt that you have done a lot of work (i.e. the 'quantity' is large), but across all your changes I come across a lot of nonsensical changes. You often only address the consequences of some problems in a completely wrong (logically flawed) way without looking for the cause of the problem and understanding it. I will do my best to fix as many sins in your code as possible, but unfortunately I will not be able to fix all of them now. I don't have time to explain everything I don't like about your code, but I don't want you to fix anything anyway (I don't feel there's much chance I'd be happy with your fixes). Just by deciding to eventually accept your code, we are incurring a lot of technical debt going forward, which I personally don't like at all, but there is no other way. Hopefully I'll have time for some major rework in the future. I would greatly appreciate it if you could take this statement professionally and not take offense. It's not personal, I don't know you, I'm just evaluating the code. But I think you should understand this assessment, because you yourself must be aware that your changes are often not well thought out. |
This reverts commit 1e0517b.
See `builder/spec/rust/rust_builder_spec.rb` for more details. This test currently fails: ```console $ rspec builder/spec/rust/rust_builder_spec.rb #### RustBuilder: initialized F Failures: 1) RustBuilder macro_expansion #parse_failed_build parses failed build information Failure/Error: expect(@builder.parse_failed_build('test_out/rust/build-1.log')).to eq [ test_nested_path, test_nested_path, ] expected: ["/mnt/c/temp/kaitai_struct/tests/builder/spec/rust/macro_expansion/spec/rust/tests/test_nested.rs", "/mnt/c/temp/kaitai_struct/tests/builder/spec/rust/macro_expansion/spec/rust/tests/test_nested.rs"] got: ["/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/macros/mod.rs", "/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/macros/mod.rs"] (compared using ==) # ./builder/spec/rust/rust_builder_spec.rb:70:in `block (4 levels) in <top (required)>' Finished in 0.03102 seconds (files took 0.14634 seconds to load) 1 example, 1 failure Failed examples: rspec ./builder/spec/rust/rust_builder_spec.rb:68 # RustBuilder macro_expansion #parse_failed_build parses failed build information ``` It will be fixed in the following commit by implementing the missing logic in `builder/rust_builder.rb`.
This change fixes the test added in the previous commit: ```console $ rspec builder/spec/rust/rust_builder_spec.rb #### RustBuilder: initialized . Finished in 0.023 seconds (files took 0.49077 seconds to load) 1 example, 0 failures ```
This breaks the YamlInts test, which is correct because it should be failing. All of the checked value instances (`test_{u4,u8}_{dec,hex}`) have type `i32` in the `compiled/rust/yaml_ints.rs` file generated by KSC, but that's wrong because none of the integer constants that these value instances should contain according to `formats/yaml_ints.ksy` fit within the `i32` range (which the Rust compiler, i.e. `rustc`, correctly reports as long as the `overflowing_literals` lint is enabled, which is the default). So the fact that this test now fails reflects reality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Agile86 @revitalyr Thank you for this PR as well.
I've had quite a bit of work with this one to be happy with it. To some extent that's fine and understandable, because obviously you can't be as familiar with the project as I am, but I still don't understand why you did certain things. For example:
-
I honestly don't understand the purpose of any of the changes to the KST translator that I reverted in 0e6254d...0ce1363. It looked like you didn't understand how to use the KST translator (in which case you could've asked), or you wanted to use it in a different workflow than what it's designed for, which is fine, but there wasn't any good reason to propose these changes in this PR because it has nothing to do with Rust specifically.
-
I don't understand why you thought that generating
assert_eq!($actStr, 0);
forexpected: 'null'
in a KST spec was OK (RustSG.scala:100
at905f1f2a
):@@ -91,11 +84,12 @@ class RustSG(spec: TestSpec, provider: ClassTypeProvider) extends BaseGenerator( override def nullAssert(actual: Ast.expr): Unit = { val actStr = translateAct(actual) finish_panic() - out.puts(s"assertNull($actStr);") + out.puts(s"assert_eq!($actStr, 0);") // TODO: Figure out what's meant to happen here }
Did you see in any other language that we would sweep nullable fields under the rug by just storing
0
in them if they have no value? I don't think so. Even the code we generate for C++ where nullable numeric fields are indistinguishable from non-nullable numeric types (unfortunately,std::optional
is only available since C++17, while we target C++98 and C++11) provides null flags so that it's at least possible to distinguish0
andnull
(although in a bit ugly way). In Rust, we should of course useOption<T>
instead.I fixed the
nullAssert
method in 8bdfd0c so that it expects the generated code to actually use theOption<T>
type for nullable fields. Currently it doesn't, so the corresponding tests will fail to build, but that's fine. Tests should accurrately represent the current state of what actually works and what doesn't. They should pass only if the features they test fully work, otherwise they should fail. There's no point in crippling the tests to make them "green" even though the features under test don't work properly - we'd just be lying to ourselves that everything is fine when it's not. -
Another example of crippling a test just to make it green was in
ExprIntDiv
- this is what it looked like before I finally regenerated it from the KST spec in16d44c9c
- https://github.com/kaitai-io/kaitai_struct_tests/blob/16d44c9c1177abd72fcfb8ffd9d17e0b86801397~/spec/rust/tests/test_expr_int_div.rs#L24-L27:assert_eq!(*r.div_pos_const().expect("error reading"), 756); assert_eq!(*r.div_neg_const().expect("error reading"), -756); assert_eq!(*r.div_pos_seq().expect("error reading"), 97130679); assert_eq!(*r.div_neg_seq().expect("error reading"), -4072);
Again, did you see
-756
and-4072
in any other language? I don't think so. I would thinkexpr_int_div.kst:8-15
is clear enough:- actual: div_pos_const expected: 756 - actual: div_neg_const expected: -757 - actual: div_pos_seq expected: 97130679 - actual: div_neg_seq expected: -4073
I'm really missing what the thought process was ("no one will notice and I'll be able to say that my implementation passes more tests?"), but please don't do this. A failing test is better than faked passing one.
-
Yet another example of crippling a test, this time using
#![allow(overflowing_literals)]
to make theYamlInts
test pass when it absolutely should not pass and the Rust compiler knows this (bf66bbe). This was actually in the compiler as well (Rust support kaitai_struct_compiler#250 (comment)) and even the runtime library had something similar (#![allow(unused)]
, which suppressed a bunch of warnings that had no good reason to be suppressed, see kaitai-io/kaitai_struct_rust_runtime@c7ce843).Come on, suppressing lints you don't like is not the solution.
-
I'm generally very skeptical about string manipulation on translated expressions (as I also mentioned in Rust support kaitai_struct_compiler#250 (review)). I'm convinced that we should always treat translated expressions as opaque strings - i.e. never attempt to "see what's inside" in any way. The reason is that string operations can't possibly understand the syntax and semantics of the expression inside the string, so it's highly unreliable, see Ruby:
enum.to_i
implementation is buggy due to string replacement kaitai_struct#1125 for instance. Whenever it's needed to "see what's inside the expression", pattern matching with AST objects (or perhaps data types) should be used.So I wasn't happy to see the string replacements that used to be in
RustSG.scala
in the KST translator. But surprisingly, I tried removing them and found out that they don't actually do anything (b0bd33e#diff-a6bcd12fc56efe3c133df903294fadf9330b65f2374740fa685a8946a45fc0b7). So yeah, I don't understand why they were there, but it doesn't matter now. -
The method you used to "expect an exception" didn't really make much sense (
test_eof_exception_bytes.rs:18-22
):if let Err(err) = res { println!("expected err: {:?}, exception: EndOfStreamError", err); } else { panic!("no expected exception: EndOfStreamError"); }
First of all, this doesn't actually test the type of the error as it should (and as it does in all other languages). Second, the
println!
is actually useless, because Rust by default doesn't display stdout of passed tests (see https://blog.ssanj.net/posts/2023-09-24-how-to-see-stdout-when-running-tests-in-rust.html). So the above is equivalent to:assert!(res.is_err(), "no expected exception: EndOfStreamError");
Which makes it even more visible that it doesn't test or indicate (so that it could be checked manually, which I don't think would be good enough, but would at least make some sense) the error type whatsoever.
Testing the error type is actually very important - if you don't do that, you're just blindly accepting any error that may arise for any reason (which is very prone to producing false negative test results, because the test might as well just crash with some exception before it even starts, but the test result will be "all good").
I've seen this before in Lua - some time ago, I decided to replace
luaunit.assertError
(which doesn't check the error type) withluaunit.assertErrorMsgMatches
in bb20b5a#diff-335bd0c62964dced90d2d661679316bb2ab4be1e36b80db89f4eaee7ed94bc83. I didn't expect this change to reveal any bug, but it did - theunable to decide endianness
error could actually never be thrown (such case was treated as big-endian), which I fixed in kaitai-io/kaitai_struct_compiler@f79666f. Andluaunit.assertError(DefaultEndianExprException.from_file, "src/endian_expr.bin")
didn't fail, because it caught an error that occurred because the arguments toDefaultEndianExprException.from_file
did not match, so no parsing actually took place - the invocation was supposed to beluaunit.assertError(DefaultEndianExprException.from_file, DefaultEndianExprException, "src/endian_expr.bin")
.
However, I managed to correct (almost) all the wrong things I found, so I'm happy with the result. I think it's quite robust now and there should be few problems with it in the future.
I honestly don't understand the purpose of any of the changes to the
KST translator that I reverted in 0e6254d...0ce1363
<0e6254d...0ce1363>.
This doesn't match the help message:
c.copy(outDir = x)
opt[Unit]('f', "force") action { (x, c) =>
c.copy(outDir = specDir)
} text(s"force overwrite specs in production spec dirs (*_default_*:
generate in $defaultOutDir)")
opt[String]('f', "force") action { (x, c) =>
and what is *_not_* default?
Message ID:
***@***.***>
|
Hello everybody,
my name is Oleh Dolhov, my collegue name is Vitaly Reshetyuk,
we are from Stroz Friedberg (AON) providing this PR to bring Rust-language support to Kaitai.
Right now we can pass almost all tests, except (4) tests, that's using features like:
Rust doesn't support inheritance, so there is no common type.
KStructUnit, tests like
nav_parent_switch_cast.ksy
,params_pass_array_struct.ksy
,params_pass_struct.ksy
.AnyType, test like
process_coerce_switch.ksy
.Please, note, you need to clone compiler and runtime/rust exactly
rust_basic_support_v2
branch.Best regards,
Oleh & Vitaly.